feat: add Claude proxy target APIs with Ollama support#171
Conversation
|
Warning Review limit reached
More reviews will be available in 22 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (41)
📝 WalkthroughWalkthroughAdds canonical Claude targetApi support and mode-aware proxy routing (responses, chat_completions, ollama); refactors CLI apply/share lifecycle to handle proxy tokens and rollback; threads targetApi through web UI state, validation, and rendering; adds version-status/update-notice flow; and expands unit and e2e tests. ChangesClaude Proxy Dual-Mode Routing with targetApi
Estimated code review effort:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/unit/web-ui-logic.test.mjs (1)
84-87: ⚡ Quick winCover canonical
chat_completionsin normalization test.This test validates two aliases but not the canonical value itself; adding it will better lock the API contract.
Proposed test addition
test('normalizeClaudeConfig accepts chat completions target api aliases', () => { + assert.strictEqual(normalizeClaudeConfig({ targetApi: 'chat_completions' }).targetApi, 'chat_completions'); assert.strictEqual(normalizeClaudeConfig({ targetApi: 'chat/completions' }).targetApi, 'chat_completions'); assert.strictEqual(normalizeClaudeConfig({ targetApi: 'chat-completions' }).targetApi, 'chat_completions'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/web-ui-logic.test.mjs` around lines 84 - 87, Add an assertion to the existing test that covers the canonical value: call normalizeClaudeConfig with targetApi: 'chat_completions' and assert its targetApi remains 'chat_completions' so the test verifies aliases plus the canonical form; update the test block in tests/unit/web-ui-logic.test.mjs around the normalizeClaudeConfig assertions to include this additional assertion referencing normalizeClaudeConfig and targetApi.tests/unit/claude-settings-sync.test.mjs (1)
480-513: ⚡ Quick winAdd a preservation case for non-default
targetApiduring merge.Current test only validates default
responses; it doesn’t catch regressions where an existingchat_completionsvalue is unintentionally reset on edit.Proposed additional assertion
test('mergeClaudeConfig preserves externalCredentialType across edits without api key', () => { @@ assert.deepStrictEqual(merged, { apiKey: '', baseUrl: 'https://api.anthropic.com/', model: 'claude-3-7-sonnet', hasKey: true, externalCredentialType: 'auth-token', targetApi: 'responses' }); + + const mergedChat = mergeClaudeConfig.call(context, { + apiKey: '', + baseUrl: 'https://api.anthropic.com', + model: 'claude-3-7-sonnet', + hasKey: true, + externalCredentialType: 'auth-token', + targetApi: 'chat_completions' + }, { + model: 'claude-sonnet-4-6' + }); + assert.strictEqual(mergedChat.targetApi, 'chat_completions'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/claude-settings-sync.test.mjs` around lines 480 - 513, The test currently only verifies that mergeClaudeConfig preserves defaults (targetApi='responses') and misses regressions where an existing non-default targetApi (e.g., 'chat_completions') is lost on merge; update the test for mergeClaudeConfig in tests/unit/claude-settings-sync.test.mjs to include an input/initial config with targetApi set to a non-default value (such as 'chat_completions') and assert that the merged result retains that explicit targetApi rather than being reset to 'responses'; use the same context.normalizeClaudeConfig and the mergeClaudeConfig function reference to craft the call and expected assertion so the test will fail if mergeClaudeConfig incorrectly overwrites non-default targetApi.tests/e2e/test-claude-proxy.js (1)
335-342: ⚡ Quick winUse predicate-based request matching instead of positional indexes.
Index-based checks can become flaky if an extra upstream chat call is introduced (retry, preflight, future hook). Matching by payload shape keeps this test stable.
Proposed assertion hardening
const upstreamChatMessages = upstream.requests.filter((item) => item.path === '/v1/chat/completions'); assert(upstreamChatMessages.length >= 2, 'claude proxy should hit upstream /v1/chat/completions'); -assert(upstreamChatMessages[0].headers.authorization === 'Bearer sk-claude-upstream', 'claude proxy chat should use provider auth for upstream'); -assert(upstreamChatMessages[0].body.messages[0].role === 'system', 'claude proxy chat should map system prompt to system message'); -assert(upstreamChatMessages[0].body.max_tokens === 128, 'claude proxy chat should map max_tokens to max_tokens'); -assert(upstreamChatMessages[0].body.stream === false, 'claude proxy chat should synthesize Anthropic streaming locally'); -assert(upstreamChatMessages[1].body.tool_choice.function.name === 'lookup', 'claude proxy chat should map tool_choice'); +const plainChatReq = upstreamChatMessages.find((item) => + item.body && item.body.model === 'DeepSeek-V4-pro' && !item.body.tools +); +const toolChatReq = upstreamChatMessages.find((item) => + item.body && Array.isArray(item.body.tools) && item.body.tools.length > 0 +); +assert(plainChatReq && plainChatReq.headers.authorization === 'Bearer sk-claude-upstream', 'claude proxy chat should use provider auth for upstream'); +assert(plainChatReq.body.messages[0].role === 'system', 'claude proxy chat should map system prompt to system message'); +assert(plainChatReq.body.max_tokens === 128, 'claude proxy chat should map max_tokens to max_tokens'); +assert(plainChatReq.body.stream === false, 'claude proxy chat should synthesize Anthropic streaming locally'); +assert(toolChatReq && toolChatReq.body.tool_choice.function.name === 'lookup', 'claude proxy chat should map tool_choice');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test-claude-proxy.js` around lines 335 - 342, The tests are using positional indexes on upstreamChatMessages which is flaky; update the assertions to locate requests by predicate on their payload shape instead of array position: use upstream.requests.filter(...) or find(...) to match by headers.authorization === 'Bearer sk-claude-upstream' for the provider-auth check, match a request whose body.messages[0].role === 'system' and body.max_tokens === 128 and body.stream === false for the system/max_tokens/stream assertions, and match a request whose body.tool_choice?.function?.name === 'lookup' for the tool_choice assertion; replace the index-based assertions that reference upstreamChatMessages[0] and [1] with these predicate-found entries and assert they exist and have the expected properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/partials/index/modals-basic.html`:
- Around line 136-143: Replace the hardcoded Chinese label, option texts and
hint for the target API selector with i18n keys rendered through t(...);
specifically update the strings inside the select block bound to
newClaudeConfig.targetApi (the label "目标 API", the two option texts "Anthropic /
OpenAI Responses" and "OpenAI Chat Completions (/v1/chat/completions)", and the
form-hint copy) to use translation keys (e.g., t('claude.targetApi.label'),
t('claude.targetApi.option.responses'),
t('claude.targetApi.option.chat_completions'), t('claude.targetApi.hint')) and
ensure the same change is applied to the duplicated block around the other
selector (the block you modified at the later occurrence). Ensure keys exist in
the locale files.
In `@web-ui/partials/index/panel-config-claude.html`:
- Line 132: The subtitle text inside the div with class "card-subtitle" (the
element gated by v-if="config.targetApi === 'chat_completions'") is hardcoded;
replace it with a localized string by calling the translation helper (e.g.,
t('panel.mode.openai_chat_completions')) so it follows the app locale, and add
the corresponding translation key to the i18n resources; ensure the
component/context provides the t(...) function before using it.
In `@web-ui/res/web-ui-render.precompiled.js`:
- Around line 1890-1894: The hardcoded UI strings shown when checking
config.targetApi (e.g., the "OpenAI Chat Completions" subtitle rendered in the
_createElementBlock with class "card-subtitle", plus the field label, option
labels and hint around the same conditional branches at the other locations)
must be replaced with i18n keys and rendered via _ctx.t(...). Update the three
affected render sites to call _ctx.t('key_name') instead of raw text, add
appropriate i18n keys for the card subtitle, field label, option labels and hint
(use descriptive keys like "card.subtitle.openai_chat_completions",
"field.label.target_api", "option.label.x", "field.hint.target_api"), and ensure
the conditional branches still pick the right key based on config.targetApi so
translations are used for language switching.
---
Nitpick comments:
In `@tests/e2e/test-claude-proxy.js`:
- Around line 335-342: The tests are using positional indexes on
upstreamChatMessages which is flaky; update the assertions to locate requests by
predicate on their payload shape instead of array position: use
upstream.requests.filter(...) or find(...) to match by headers.authorization ===
'Bearer sk-claude-upstream' for the provider-auth check, match a request whose
body.messages[0].role === 'system' and body.max_tokens === 128 and body.stream
=== false for the system/max_tokens/stream assertions, and match a request whose
body.tool_choice?.function?.name === 'lookup' for the tool_choice assertion;
replace the index-based assertions that reference upstreamChatMessages[0] and
[1] with these predicate-found entries and assert they exist and have the
expected properties.
In `@tests/unit/claude-settings-sync.test.mjs`:
- Around line 480-513: The test currently only verifies that mergeClaudeConfig
preserves defaults (targetApi='responses') and misses regressions where an
existing non-default targetApi (e.g., 'chat_completions') is lost on merge;
update the test for mergeClaudeConfig in
tests/unit/claude-settings-sync.test.mjs to include an input/initial config with
targetApi set to a non-default value (such as 'chat_completions') and assert
that the merged result retains that explicit targetApi rather than being reset
to 'responses'; use the same context.normalizeClaudeConfig and the
mergeClaudeConfig function reference to craft the call and expected assertion so
the test will fail if mergeClaudeConfig incorrectly overwrites non-default
targetApi.
In `@tests/unit/web-ui-logic.test.mjs`:
- Around line 84-87: Add an assertion to the existing test that covers the
canonical value: call normalizeClaudeConfig with targetApi: 'chat_completions'
and assert its targetApi remains 'chat_completions' so the test verifies aliases
plus the canonical form; update the test block in
tests/unit/web-ui-logic.test.mjs around the normalizeClaudeConfig assertions to
include this additional assertion referencing normalizeClaudeConfig and
targetApi.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ae0fbd9-703c-4ef4-b64e-cefdd78b6682
📒 Files selected for processing (13)
cli.jscli/claude-proxy.jstests/e2e/test-claude-proxy.jstests/unit/claude-proxy-adapter.test.mjstests/unit/claude-settings-sync.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/app.jsweb-ui/logic.claude.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/res/web-ui-render.precompiled.js
📜 Review details
🔇 Additional comments (10)
cli.js (1)
293-294: LGTM!Also applies to: 5492-5494
cli/claude-proxy.js (2)
136-151: LGTM!Also applies to: 238-352, 432-505, 1268-1270
644-647: LGTM!Also applies to: 676-683, 757-784, 810-813, 1015-1015, 1041-1041, 1074-1077, 1080-1084, 1097-1099, 1198-1199, 1247-1247
web-ui/logic.claude.mjs (1)
72-75: LGTM!Also applies to: 82-83
web-ui/app.js (1)
278-279: LGTM!Also applies to: 284-285, 292-294, 539-542
web-ui/modules/app.methods.claude-config.mjs (1)
55-57: LGTM!Also applies to: 67-69, 88-88, 110-110, 186-186, 208-210
web-ui/modules/app.methods.startup-claude.mjs (1)
250-252: LGTM!tests/unit/claude-proxy-adapter.test.mjs (1)
7-10: LGTM!Also applies to: 67-105, 145-172
tests/unit/web-ui-logic.test.mjs (1)
54-55: LGTM!Also applies to: 66-80
tests/e2e/test-claude-proxy.js (1)
101-142: LGTM!Also applies to: 260-334, 343-394
|
Updated the PR to close the Claude Chat Completions apply-path gap and the UI i18n review comment. Changes in this push:
Validation run locally:
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli.js (1)
9176-9214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback proxy runtime on post-start failure paths.
If the proxy starts successfully but a later step fails (e.g., reading/writing Claude settings), the function returns an error while leaving the proxy running. That creates inconsistent state (
applyfailed, runtime still active).Proposed fix
async function applyToClaudeSettings(config = {}) { + let proxyStarted = false; try { @@ if (targetApi === 'chat_completions') { await stopBuiltinClaudeProxyRuntime(); proxyResult = await startBuiltinClaudeProxyRuntime({ @@ if (!proxyResult || proxyResult.error || proxyResult.success === false || !proxyResult.listenUrl) { + await stopBuiltinClaudeProxyRuntime(); return { success: false, mode: 'claude-proxy', error: (proxyResult && proxyResult.error) || '启动 Claude 兼容代理失败' }; } + proxyStarted = true; settingsBaseUrl = proxyResult.listenUrl; settingsApiKey = 'codexmate'; } else { @@ const readResult = readJsonObjectFromFile(CLAUDE_SETTINGS_FILE, {}); if (!readResult.ok) { + if (proxyStarted) { + await stopBuiltinClaudeProxyRuntime(); + } return { success: false, mode: 'settings-file', error: readResult.error }; } @@ } catch (e) { + if (proxyStarted) { + try { await stopBuiltinClaudeProxyRuntime(); } catch (_) { } + } return { success: false, mode: 'settings-file', error: e.message || '应用 Claude 配置失败' }; } }Also applies to: 9262-9267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli.js` around lines 9176 - 9214, The code may leave the built-in Claude proxy running if later steps fail; ensure we stop the proxy on all post-start failure paths by calling stopBuiltinClaudeProxyRuntime() before any early return after a successful start (i.e., when proxyResult indicates success and you later hit an error while handling CLAUDE_SETTINGS_FILE, readJsonObjectFromFile, or writeJsonAtomic). Specifically, after using startBuiltinClaudeProxyRuntime() and assigning settingsBaseUrl/settingsApiKey (and likewise in the branch that writes BUILTIN_CLAUDE_PROXY_SETTINGS_FILE), add logic to call stopBuiltinClaudeProxyRuntime() (or otherwise tear down the runtime) whenever you are about to return an error, so proxyResult/started runtimes are cleaned up consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli.js`:
- Around line 9178-9197: The code currently sets a predictable proxy credential
(settingsApiKey = 'codexmate') after calling startBuiltinClaudeProxyRuntime,
which makes the proxy guessable if bound to a non-loopback address; replace this
by generating a cryptographically-secure random token for settingsApiKey (e.g.,
using crypto.randomBytes) and assign that token instead of the constant, and
additionally validate proxyResult.listenUrl (from
startBuiltinClaudeProxyRuntime) to ensure it is bound to a loopback address
(127.0.0.1/::1); if the listenUrl is non-loopback either refuse to use a
fixed/predictable key (fail or require explicit config) or log and rotate a
random token, ensuring settingsBaseUrl uses proxyResult.listenUrl and only the
random token is exposed as the API key.
---
Outside diff comments:
In `@cli.js`:
- Around line 9176-9214: The code may leave the built-in Claude proxy running if
later steps fail; ensure we stop the proxy on all post-start failure paths by
calling stopBuiltinClaudeProxyRuntime() before any early return after a
successful start (i.e., when proxyResult indicates success and you later hit an
error while handling CLAUDE_SETTINGS_FILE, readJsonObjectFromFile, or
writeJsonAtomic). Specifically, after using startBuiltinClaudeProxyRuntime() and
assigning settingsBaseUrl/settingsApiKey (and likewise in the branch that writes
BUILTIN_CLAUDE_PROXY_SETTINGS_FILE), add logic to call
stopBuiltinClaudeProxyRuntime() (or otherwise tear down the runtime) whenever
you are about to return an error, so proxyResult/started runtimes are cleaned up
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1c4589c-b7ff-4435-a393-681c33a08def
📒 Files selected for processing (9)
cli.jscli/claude-proxy.jstests/e2e/test-claude.jstests/unit/web-ui-logic.test.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/res/web-ui-render.precompiled.js
✅ Files skipped from review due to trivial changes (1)
- web-ui/modules/i18n.dict.mjs
📜 Review details
🔇 Additional comments (22)
tests/e2e/test-claude.js (1)
67-91: LGTM!web-ui/partials/index/panel-config-claude.html (1)
132-132: Ensureclaude.targetApi.chatCompletionsBadgeis present in i18n resources.
claude.targetApi.chatCompletionsBadgeis defined inweb-ui/modules/i18n.dict.mjsfor all supported locale blocks (zh,ja,en), so thet('claude.targetApi.chatCompletionsBadge')call won’t fail due to a missing key.web-ui/res/web-ui-render.precompiled.js (1)
1894-1894: LGTM!Also applies to: 5561-5571, 5675-5685
tests/unit/web-ui-logic.test.mjs (1)
84-88: LGTM!web-ui/partials/index/modals-basic.html (1)
136-143: LGTM!Also applies to: 175-182
cli.js (1)
9154-9159: LGTM!Also applies to: 9399-9403, 11008-11008, 15556-15558
cli/claude-proxy.js (10)
136-151: LGTM!
238-293: LGTM!
295-352: LGTM!
432-505: LGTM!
816-840: LGTM!
1212-1212: LGTM!
1099-1111: LGTM!
526-617: LGTM!
681-684: LGTM!
757-785: LGTM!web-ui/modules/app.methods.claude-config.mjs (6)
50-59: LGTM!
61-72: LGTM!
85-89: LGTM!
110-127: LGTM!
186-199: LGTM!
202-211: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli.js (1)
9176-9267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop the proxy again if the later settings write path fails.
After Line 9178 starts the runtime, any later failure in
readJsonObjectFromFile,backupFileIfNeededOnce, orwriteJsonAtomicreturns an error but never tears the proxy back down. That leaves a leaked background process and persisted proxy state that no longer matches~/.claude/settings.json.🧯 Suggested rollback
async function applyToClaudeSettings(config = {}) { try { const apiKey = (config.apiKey || '').trim(); if (!apiKey) { return { success: false, mode: 'settings-file', error: '请先输入 API Key' }; @@ const targetApi = normalizeClaudeTargetApi(config.targetApi); let settingsBaseUrl = baseUrl; let settingsApiKey = apiKey; let proxyResult = null; + let proxyStarted = false; @@ proxyResult = await startBuiltinClaudeProxyRuntime({ enabled: true, provider: typeof config.name === 'string' ? config.name.trim() : '', authSource: 'provider', targetApi, @@ if (!proxyResult || proxyResult.error || proxyResult.success === false || !proxyResult.listenUrl) { return { success: false, mode: 'claude-proxy', error: (proxyResult && proxyResult.error) || '启动 Claude 兼容代理失败' }; } + proxyStarted = true; settingsBaseUrl = proxyResult.listenUrl; settingsApiKey = 'codexmate'; @@ } catch (e) { + if (proxyStarted) { + try { + await stopBuiltinClaudeProxyRuntime(); + } catch (_) { } + } return { success: false, mode: 'settings-file', error: e.message || '应用 Claude 配置失败' };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli.js` around lines 9176 - 9267, The code can leak a running proxy if startBuiltinClaudeProxyRuntime succeeds but later steps (readJsonObjectFromFile, backupFileIfNeededOnce, writeJsonAtomic) fail; modify the flow around startBuiltinClaudeProxyRuntime/stopBuiltinClaudeProxyRuntime so that when proxyResult indicates a proxy was started you always stop it on any early return or exception—either wrap the post-proxy logic in a try/finally that calls stopBuiltinClaudeProxyRuntime() in the finally, or track a boolean like proxyStarted and call stopBuiltinClaudeProxyRuntime() before each error return and in the catch block; ensure this covers the branches that return after readJsonObjectFromFile errors and after writeJsonAtomic/backup failures, and still preserve returning proxy info only when the operation completes successfully.
♻️ Duplicate comments (1)
cli.js (1)
9178-9196:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForce loopback before writing the fixed proxy token.
startBuiltinClaudeProxyRuntime(...)is called without a host override, so a previously persisted non-loopback bind can still be reused here. Line 9196 then writes a constantANTHROPIC_API_KEY, which makes the proxy credential guessable off-box if this runtime comes up anywhere other than loopback.🔒 Minimal hardening
proxyResult = await startBuiltinClaudeProxyRuntime({ enabled: true, + host: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.host, provider: typeof config.name === 'string' ? config.name.trim() : '', authSource: 'provider', targetApi, timeoutMs: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.timeoutMs, upstreamProviderName: typeof config.name === 'string' ? config.name.trim() : '', upstreamBaseUrl: baseUrl, upstreamApiKey: apiKey }); if (!proxyResult || proxyResult.error || proxyResult.success === false || !proxyResult.listenUrl) { return { success: false, mode: 'claude-proxy', error: (proxyResult && proxyResult.error) || '启动 Claude 兼容代理失败' }; } + { + const listenHost = new URL(proxyResult.listenUrl).hostname; + if (listenHost !== '127.0.0.1' && listenHost !== 'localhost' && listenHost !== '::1') { + await stopBuiltinClaudeProxyRuntime(); + return { + success: false, + mode: 'claude-proxy', + error: 'Claude 兼容代理在 chat_completions 模式下必须仅监听 loopback' + }; + } + } settingsBaseUrl = proxyResult.listenUrl; settingsApiKey = 'codexmate';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli.js` around lines 9178 - 9196, The code currently starts the Claude proxy without forcing a loopback bind and then writes a fixed proxy API key ('codexmate'/ANTHROPIC_API_KEY), which can leak if the runtime binds non-loopback; update the call to startBuiltinClaudeProxyRuntime to force a loopback host (e.g., pass bindHost: '127.0.0.1' or equivalent) and only persist or expose a generated/ephemeral settingsApiKey when proxyResult.listenUrl is loopback; replace the constant 'codexmate' assignment to settingsApiKey with a securely generated random token (or skip persisting the token) when proxyResult.listenUrl is not loopback so the credential cannot be guessed off-box (references: startBuiltinClaudeProxyRuntime, settingsApiKey, ANTHROPIC_API_KEY).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/claude-proxy.js`:
- Around line 769-776: resolveOpenaiBridgeUpstream may return null/undefined
causing a TypeError when accessing bridgeUpstream.baseUrl; update the logic
around resolveOpenaiBridgeUpstream (the bridgeUpstream variable) to explicitly
check for null/undefined and return an error if so, then only access
bridgeUpstream.baseUrl (used to set bridgeBaseUrl) after confirming
bridgeUpstream is an object and has a baseUrl property; keep existing
isValidHttpUrl(providerName) check and error messages using providerName
unchanged.
---
Outside diff comments:
In `@cli.js`:
- Around line 9176-9267: The code can leak a running proxy if
startBuiltinClaudeProxyRuntime succeeds but later steps (readJsonObjectFromFile,
backupFileIfNeededOnce, writeJsonAtomic) fail; modify the flow around
startBuiltinClaudeProxyRuntime/stopBuiltinClaudeProxyRuntime so that when
proxyResult indicates a proxy was started you always stop it on any early return
or exception—either wrap the post-proxy logic in a try/finally that calls
stopBuiltinClaudeProxyRuntime() in the finally, or track a boolean like
proxyStarted and call stopBuiltinClaudeProxyRuntime() before each error return
and in the catch block; ensure this covers the branches that return after
readJsonObjectFromFile errors and after writeJsonAtomic/backup failures, and
still preserve returning proxy info only when the operation completes
successfully.
---
Duplicate comments:
In `@cli.js`:
- Around line 9178-9196: The code currently starts the Claude proxy without
forcing a loopback bind and then writes a fixed proxy API key
('codexmate'/ANTHROPIC_API_KEY), which can leak if the runtime binds
non-loopback; update the call to startBuiltinClaudeProxyRuntime to force a
loopback host (e.g., pass bindHost: '127.0.0.1' or equivalent) and only persist
or expose a generated/ephemeral settingsApiKey when proxyResult.listenUrl is
loopback; replace the constant 'codexmate' assignment to settingsApiKey with a
securely generated random token (or skip persisting the token) when
proxyResult.listenUrl is not loopback so the credential cannot be guessed
off-box (references: startBuiltinClaudeProxyRuntime, settingsApiKey,
ANTHROPIC_API_KEY).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f3075cd-6ca8-427d-85f1-4f2a07df91b4
📒 Files selected for processing (15)
cli.jscli/claude-proxy.jstests/e2e/test-claude-proxy.jstests/e2e/test-claude.jstests/unit/claude-proxy-adapter.test.mjstests/unit/claude-settings-sync.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/app.jsweb-ui/logic.claude.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/res/web-ui-render.precompiled.js
📜 Review details
🔇 Additional comments (22)
web-ui/partials/index/panel-config-claude.html (1)
132-132: LGTM!web-ui/logic.claude.mjs (1)
72-75: LGTM!Also applies to: 82-83
web-ui/app.js (1)
278-278: LGTM!Also applies to: 284-284, 292-294, 539-542
web-ui/partials/index/modals-basic.html (1)
136-143: LGTM!Also applies to: 175-182
web-ui/modules/i18n.dict.mjs (1)
1051-1055: LGTM!Also applies to: 2112-2116, 3183-3187
tests/unit/claude-settings-sync.test.mjs (1)
490-492: LGTM!Also applies to: 511-513
tests/unit/claude-proxy-adapter.test.mjs (1)
7-7: LGTM!Also applies to: 9-9, 67-105, 145-172
tests/unit/web-ui-logic.test.mjs (1)
54-55: LGTM!Also applies to: 67-68, 79-80, 84-88
tests/e2e/test-claude.js (1)
67-84: LGTM!Also applies to: 90-92
tests/e2e/test-claude-proxy.js (1)
101-142: LGTM!Also applies to: 260-384, 392-394
web-ui/modules/app.methods.startup-claude.mjs (1)
250-252: LGTM!web-ui/res/web-ui-render.precompiled.js (1)
1890-1900: LGTM!Also applies to: 5560-5572, 5674-5686
web-ui/modules/app.methods.claude-config.mjs (1)
55-56: LGTM!Also applies to: 67-68, 88-88, 110-110, 112-112, 186-186, 188-188, 208-209
cli.js (1)
293-294: LGTM!Also applies to: 5492-5494, 9154-9159, 9399-9403, 11007-11008, 15556-15564
cli/claude-proxy.js (8)
136-151: LGTM!
238-293: LGTM!
295-352: LGTM!
432-505: LGTM!
644-695: LGTM!
816-839: LGTM!
1100-1125: LGTM!
1224-1224: LGTM!Also applies to: 1273-1273, 1294-1296
|
Pushed a follow-up hardening commit for the latest CodeRabbit actionable comments: Changes:
Validation run locally after this commit:
@coderabbitai re-review |
|
Triggering a full re-review now. 🐇✨ ✅ Actions performedFull review triggered. |
|
Systematic test pass added for the Claude What changed in this push:
Validation run locally:
Additional real-upstream smoke:
@coderabbitai re-review |
|
Triggering a full re-review now. 🐇✨ ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli.js (1)
9216-9251:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelay proxy shutdown until the settings write succeeds.
In the
responsesbranch, Lines 9217-9218 tear down the existing proxy beforeCLAUDE_SETTINGS_FILEis even read. If the later read/write fails, apply returns an error but the previously working proxy is already gone.Suggested fix
if (targetApi === 'chat_completions') { await stopBuiltinClaudeProxyRuntime(); const proxyToken = crypto.randomBytes(24).toString('hex'); proxyResult = await startBuiltinClaudeProxyRuntime({ enabled: true, host: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.host, provider: typeof config.name === 'string' ? config.name.trim() : '', authSource: 'provider', targetApi, timeoutMs: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.timeoutMs, upstreamProviderName: typeof config.name === 'string' ? config.name.trim() : '', upstreamBaseUrl: baseUrl, upstreamApiKey: apiKey }); if (!proxyResult || proxyResult.error || proxyResult.success === false || !proxyResult.listenUrl) { await stopBuiltinClaudeProxyRuntime(); resetBuiltinClaudeProxySavedSettingsToResponses(); return { success: false, mode: 'claude-proxy', error: (proxyResult && proxyResult.error) || '启动 Claude 兼容代理失败' }; } proxyStarted = true; settingsBaseUrl = proxyResult.listenUrl; settingsApiKey = proxyToken; - } else { - await stopBuiltinClaudeProxyRuntime(); - resetBuiltinClaudeProxySavedSettingsToResponses(); } @@ ensureDir(CLAUDE_DIR); const backupPath = backupFileIfNeededOnce(CLAUDE_SETTINGS_FILE); writeJsonAtomic(CLAUDE_SETTINGS_FILE, nextSettings); + if (targetApi !== 'chat_completions') { + await stopBuiltinClaudeProxyRuntime(); + resetBuiltinClaudeProxySavedSettingsToResponses(); + }
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/test-claude-proxy.js`:
- Around line 356-370: Wrap the block that mutates and tests the bridge settings
in a try...finally so the original file is always restored: move the
fs.writeFileSync(bridgeSettingsPath, JSON.stringify({ providers: {} }, ...)) and
the subsequent api('claude-proxy-start') / asserts into a try block and put
fs.writeFileSync(bridgeSettingsPath, savedBridgeSettings, 'utf-8') inside a
finally block; reference the bridgeSettingsPath and savedBridgeSettings
variables and ensure the api calls missingBridgeStartResult and
missingBridgeStatus remain inside the try so any thrown error still triggers the
finally restoration.
In `@tests/e2e/test-claude.js`:
- Around line 99-109: The test corrupts the Claude settings.json
(claudeSettingsPath) but only restores it at the end, so failures leave the file
corrupted; wrap the mutation and subsequent assertions (the call to
api('apply-claude-config') and the checks against failedChatApply and
claudeProxyStatusAfterFailedApply from api('claude-proxy-status')) in a
try/finally and always write back validClaudeSettings to claudeSettingsPath in
the finally block to guarantee restoration even if an assertion throws.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ad0bbee-40c7-483e-83ae-36c3bd62ac49
📒 Files selected for processing (4)
cli.jscli/claude-proxy.jstests/e2e/test-claude-proxy.jstests/e2e/test-claude.js
📜 Review details
🔇 Additional comments (3)
cli.js (2)
9161-9172: LGTM!
9192-9215: ⚡ Quick winRemove the “keep proxyToken and ANTHROPIC_API_KEY in sync” requirement
proxyTokenis only generated incli.jsand written toenv.ANTHROPIC_API_KEY, butstartBuiltinClaudeProxyRuntime(...)is called without it. The proxy’s incoming authentication is driven byprocess.env.CODEXMATE_HTTP_TOKENand checksAuthorization/x-codexmate-tokenheaders (notx-api-key/ANTHROPIC_API_KEY), so Claude’sANTHROPIC_API_KEYvalue doesn’t need to matchproxyTokenfor the proxy to accept requests.> Likely an incorrect or invalid review comment.cli/claude-proxy.js (1)
769-771: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/app.js (1)
534-543:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard malformed persisted Claude config entries before assigning
targetApi.On Line 540,
config.targetApi = ...assumesconfigis an object. Corrupted localStorage entries can throw here and break startup normalization.Suggested fix
for (const [name, config] of Object.entries(this.claudeConfigs)) { + if (!config || typeof config !== 'object' || Array.isArray(config)) { + delete this.claudeConfigs[name]; + continue; + } if (config.apiKey && config.apiKey.includes('****')) { config.apiKey = ''; config.hasKey = false; } const targetApiRaw = typeof config.targetApi === 'string' ? config.targetApi.trim().toLowerCase() : ''; config.targetApi = targetApiRaw === 'chat_completions' || targetApiRaw === 'chat-completions' || targetApiRaw === 'chat/completions' ? 'chat_completions' : 'responses'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/app.js` around lines 534 - 543, The loop over this.claudeConfigs assumes each entry is a valid object; guard against malformed persisted entries by checking that config is a non-null object before reading or writing properties like config.apiKey, config.hasKey and config.targetApi, and if an entry is invalid either skip normalization or replace it with a safe default object (e.g., { apiKey: '', hasKey: false, targetApi: 'responses' }) so the subsequent assignment to config.targetApi cannot throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli.js`:
- Around line 9183-9203: The code allows targetApi === 'chat_completions' to
startBuiltinClaudeProxyRuntime even when config.baseUrl is empty (falling back
to the Anthropic Messages URL), which silently wires the proxy to the wrong
upstream; update the validation around normalizeClaudeTargetApi/targetApi and
baseUrl before calling startBuiltinClaudeProxyRuntime so that if targetApi ===
'chat_completions' and config.baseUrl is missing or equal to the default
Anthropic messages URL you either throw/return a validation error or call
startBuiltinClaudeProxyRuntime without upstreamBaseUrl (omit the upstreamBaseUrl
property) so the runtime resolves the upstream from config.name; adjust the
block that prepares proxyResult (the call to startBuiltinClaudeProxyRuntime and
the upstreamBaseUrl/upstreamProviderName fields) accordingly to enforce this
check.
- Around line 9192-9203: The generated proxyToken from
applyToClaudeSettings(...) is not passed into startBuiltinClaudeProxyRuntime,
causing an auth mismatch; update the call to startBuiltinClaudeProxyRuntime (the
invocation that currently sets host, provider, authSource, targetApi, timeoutMs,
upstreamProviderName, upstreamBaseUrl, upstreamApiKey) to include the generated
proxyToken (e.g., as an authToken or proxyToken parameter) so the runtime
enforces the same token that was written into Claude’s ANTHROPIC_API_KEY, and
also verify cli/claude-proxy.js checks that token (or falls back to
CODEXMATE_HTTP_TOKEN only when appropriate) for non-loopback requests when
DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.host is non-loopback. Ensure parameter
naming matches startBuiltinClaudeProxyRuntime’s signature and update any
doc/comment accordingly.
---
Outside diff comments:
In `@web-ui/app.js`:
- Around line 534-543: The loop over this.claudeConfigs assumes each entry is a
valid object; guard against malformed persisted entries by checking that config
is a non-null object before reading or writing properties like config.apiKey,
config.hasKey and config.targetApi, and if an entry is invalid either skip
normalization or replace it with a safe default object (e.g., { apiKey: '',
hasKey: false, targetApi: 'responses' }) so the subsequent assignment to
config.targetApi cannot throw.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 858ec6da-e7ea-42fc-b114-09db6512dd0d
📒 Files selected for processing (15)
cli.jscli/claude-proxy.jstests/e2e/test-claude-proxy.jstests/e2e/test-claude.jstests/unit/claude-proxy-adapter.test.mjstests/unit/claude-settings-sync.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/app.jsweb-ui/logic.claude.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/i18n.dict.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/res/web-ui-render.precompiled.js
📜 Review details
🔇 Additional comments (9)
cli.js (1)
293-294: LGTM!Also applies to: 5492-5494, 9154-9172, 9417-9421, 11025-11026
tests/e2e/test-claude-proxy.js (1)
356-370: Always restore bridge settings withtry/finally.Line 358 mutates
codexmate-openai-bridge.json, but restore on Line 370 is not guaranteed if an assertion or API call fails first.tests/e2e/test-claude.js (1)
99-109: Wrap Claude settings corruption/restore intry/finally.Line 101 corrupts
settings.json, but restoration on Line 109 is skipped if intermediate assertions fail.web-ui/modules/app.methods.claude-config.mjs (1)
55-56: LGTM!Also applies to: 67-68, 88-88, 110-113, 186-189, 208-209
web-ui/modules/app.methods.startup-claude.mjs (1)
250-251: LGTM!web-ui/modules/i18n.dict.mjs (1)
1051-1055: LGTM!Also applies to: 2112-2116, 3183-3187
web-ui/partials/index/modals-basic.html (1)
136-143: LGTM!Also applies to: 175-182
web-ui/partials/index/panel-config-claude.html (1)
132-132: LGTM!web-ui/res/web-ui-render.precompiled.js (1)
1890-1900: LGTM!Also applies to: 5560-5572, 5674-5686
…letions-proxy # Conflicts: # cli.js # web-ui/modules/app.methods.claude-config.mjs # web-ui/modules/i18n.dict.mjs # web-ui/res/web-ui-render.precompiled.js
|
Resolved the What changed in this push:
Local validation run on Node 22.22.1:
@coderabbitai re-review |
|
Triggering a full re-review now. 🐇✨ ✅ Actions performedFull review triggered. |
|
Minimized the PR diff with No runtime code changed in this follow-up. The effective PR diff is now 18 files, limited to the Claude Chat Completions proxy path, required Web UI/i18n integration, generated render output, and regression tests. Previous local validation still applies for the code-bearing merge resolution:
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
|
Updated this PR with the full follow-up pass:
Validation actually run:
@coderabbitai review |
|
The test count growth (533 → 583) and the real Ollama smoke test (including the ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli.js (1)
15816-15829:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the MCP schema match the runtime's
ollamarules.
applyToClaudeSettings()allowstargetApi: 'ollama'without an API key, but this MCP tool still requiresapiKeyunconditionally. That blocks the new Ollama mode for MCP clients even though the backend supports it.Suggested fix
inputSchema: { - type: 'object', - properties: { - apiKey: { type: 'string' }, - baseUrl: { type: 'string' }, - model: { type: 'string' }, - name: { type: 'string' }, - targetApi: { type: 'string' } - }, - required: ['apiKey'], - additionalProperties: false + oneOf: [ + { + type: 'object', + properties: { + baseUrl: { type: 'string' }, + model: { type: 'string' }, + name: { type: 'string' }, + targetApi: { enum: ['ollama'] } + }, + additionalProperties: false + }, + { + type: 'object', + properties: { + apiKey: { type: 'string' }, + baseUrl: { type: 'string' }, + model: { type: 'string' }, + name: { type: 'string' }, + targetApi: { type: 'string' } + }, + required: ['apiKey'], + additionalProperties: false + } + ] },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli.js` around lines 15816 - 15829, The MCP tool's inputSchema for action 'codexmate.claude.config.apply' currently requires apiKey unconditionally, which conflicts with applyToClaudeSettings() that allows targetApi: 'ollama' without an API key; update the schema so apiKey is optional by removing it from the top-level required array and add a JSON Schema conditional (if/then) that only requires apiKey when targetApi is not 'ollama' (or equivalently, require apiKey when targetApi equals the non-ollama value(s)); reference the inputSchema object for 'codexmate.claude.config.apply' and the applyToClaudeSettings() behavior when making this change.
♻️ Duplicate comments (1)
cli.js (1)
9360-9378:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
chat_completionshas no OpenAI-compatible upstream.
targetApi === 'chat_completions'still falls back to the Anthropic Messages base URL and passes it asupstreamBaseUrl. That silently starts the proxy against the wrong wire API instead of rejecting the config or letting runtime-side provider resolution take over.Suggested fix
- const baseUrl = (config.baseUrl || (targetApi === 'ollama' ? 'http://127.0.0.1:11434' : 'https://open.bigmodel.cn/api/anthropic')).trim(); + const configuredBaseUrl = typeof config.baseUrl === 'string' ? config.baseUrl.trim() : ''; + const baseUrl = (configuredBaseUrl || (targetApi === 'ollama' ? 'http://127.0.0.1:11434' : 'https://open.bigmodel.cn/api/anthropic')).trim(); const model = (config.model || DEFAULT_CLAUDE_MODEL).trim(); let settingsBaseUrl = baseUrl; let settingsApiKey = apiKey; let proxyResult = null; if (targetApi === 'chat_completions' || targetApi === 'ollama') { + if (targetApi === 'chat_completions' && !configuredBaseUrl && !(typeof config.name === 'string' && config.name.trim())) { + return { success: false, mode: 'claude-proxy', error: 'chat_completions 模式需要显式的上游 Base URL 或可解析的 provider 名称' }; + } await stopBuiltinClaudeProxyRuntime(); const proxyToken = crypto.randomBytes(24).toString('hex'); proxyResult = await startBuiltinClaudeProxyRuntime({ enabled: true, host: DEFAULT_BUILTIN_CLAUDE_PROXY_SETTINGS.host, @@ - upstreamBaseUrl: baseUrl, + ...(configuredBaseUrl ? { upstreamBaseUrl: configuredBaseUrl } : {}), upstreamApiKey: apiKey });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli.js` around lines 9360 - 9378, When targetApi === 'chat_completions' the code currently uses the Anthropic default baseUrl and blindly starts the built-in proxy with that upstreamBaseUrl; instead, in the block that calls stopBuiltinClaudeProxyRuntime() and startBuiltinClaudeProxyRuntime() validate that baseUrl (or config.baseUrl) is OpenAI-compatible before starting the proxy. If the upstream is not OpenAI-compatible, reject/fail fast (throw or return an error) or skip starting the proxy and allow runtime-side provider resolution to proceed; update the logic around targetApi, upstreamBaseUrl, and startBuiltinClaudeProxyRuntime to either set upstreamBaseUrl only when it's OpenAI-compatible or to error out when targetApi === 'chat_completions' and no OpenAI-compatible upstream is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cli.js`:
- Around line 15816-15829: The MCP tool's inputSchema for action
'codexmate.claude.config.apply' currently requires apiKey unconditionally, which
conflicts with applyToClaudeSettings() that allows targetApi: 'ollama' without
an API key; update the schema so apiKey is optional by removing it from the
top-level required array and add a JSON Schema conditional (if/then) that only
requires apiKey when targetApi is not 'ollama' (or equivalently, require apiKey
when targetApi equals the non-ollama value(s)); reference the inputSchema object
for 'codexmate.claude.config.apply' and the applyToClaudeSettings() behavior
when making this change.
---
Duplicate comments:
In `@cli.js`:
- Around line 9360-9378: When targetApi === 'chat_completions' the code
currently uses the Anthropic default baseUrl and blindly starts the built-in
proxy with that upstreamBaseUrl; instead, in the block that calls
stopBuiltinClaudeProxyRuntime() and startBuiltinClaudeProxyRuntime() validate
that baseUrl (or config.baseUrl) is OpenAI-compatible before starting the proxy.
If the upstream is not OpenAI-compatible, reject/fail fast (throw or return an
error) or skip starting the proxy and allow runtime-side provider resolution to
proceed; update the logic around targetApi, upstreamBaseUrl, and
startBuiltinClaudeProxyRuntime to either set upstreamBaseUrl only when it's
OpenAI-compatible or to error out when targetApi === 'chat_completions' and no
OpenAI-compatible upstream is provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8710ccd6-8260-45da-8a0b-5de00b139384
📒 Files selected for processing (21)
cli.jscli/claude-proxy.jstests/e2e/test-claude-proxy.jstests/e2e/test-claude.jstests/unit/claude-proxy-adapter.test.mjstests/unit/claude-settings-sync.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/app.jsweb-ui/logic.claude.mjsweb-ui/logic.runtime.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.session-timeline.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/i18n/locales/en.mjsweb-ui/modules/i18n/locales/ja.mjsweb-ui/modules/i18n/locales/vi.mjsweb-ui/modules/i18n/locales/zh.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/controls-forms.css
💤 Files with no reviewable changes (3)
- web-ui/modules/app.methods.session-timeline.mjs
- web-ui/res/web-ui-render.precompiled.js
- web-ui/logic.runtime.mjs
✅ Files skipped from review due to trivial changes (3)
- web-ui/modules/i18n/locales/zh.mjs
- tests/unit/claude-settings-sync.test.mjs
- web-ui/modules/i18n/locales/ja.mjs
🚧 Files skipped from review as they are similar to previous changes (9)
- web-ui/partials/index/panel-config-claude.html
- web-ui/app.js
- web-ui/modules/i18n/locales/vi.mjs
- web-ui/modules/app.methods.startup-claude.mjs
- web-ui/logic.claude.mjs
- web-ui/partials/index/modals-basic.html
- web-ui/modules/app.methods.claude-config.mjs
- tests/e2e/test-claude.js
- tests/e2e/test-claude-proxy.js
📜 Review details
🔇 Additional comments (20)
web-ui/modules/i18n/locales/en.mjs (1)
1094-1100: LGTM!tests/unit/web-ui-logic.test.mjs (1)
514-525: LGTM!Also applies to: 982-983, 1003-1004
web-ui/styles/controls-forms.css (1)
409-413: LGTM!Also applies to: 441-532
cli/claude-proxy.js (12)
87-114: LGTM!
135-141: LGTM!Also applies to: 163-165
277-364: LGTM!
366-423: LGTM!
427-538: LGTM!
693-740: LGTM!
916-921: LGTM!
995-1024: LGTM!
1056-1082: LGTM!
1283-1284: LGTM!Also applies to: 1307-1307
1342-1373: LGTM!
1472-1472: LGTM!Also applies to: 1521-1521, 1543-1546
tests/unit/claude-proxy-adapter.test.mjs (5)
1-14: LGTM!
69-90: LGTM!
132-169: LGTM!
171-227: LGTM!
296-317: LGTM!
awsl233777
changed the title
|
Follow-up E2E pass completed after the previous update. What the end-to-end run covered:
The E2E run caught a real bug: Ollama upstream paths were being built as Validation after the fix:
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
# Conflicts: # web-ui/modules/app.methods.claude-config.mjs # web-ui/modules/i18n/locales/vi.mjs
|
Resolved the merge conflicts with Conflict files handled:
Validation run locally after resolving conflicts:
GitHub now reports the PR as mergeable ( @coderabbitai re-review |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/claude-proxy-adapter.test.mjs (1)
409-417:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEliminate TOCTOU port probing to prevent flaky
EADDRINUSEfailures.Line 409–Line 417 finds a free port and releases it before
startBuiltinClaudeProxyRuntimebinds it (e.g., Line 449, Line 537, Line 616), which creates a race window. Please switch to a retry-on-EADDRINUSEwrapper for startup (or supportport: 0in runtime and assert the returnedlistenUrl).💡 Minimal test-side fix (retry wrapper)
+async function startProxyWithRetry(controller, payload, attempts = 4) { + let last = null; + for (let i = 0; i < attempts; i += 1) { + const result = await controller.startBuiltinClaudeProxyRuntime(payload); + if (result && result.success) return result; + const msg = result && result.error ? String(result.error) : ''; + if (!/EADDRINUSE/i.test(msg) || i === attempts - 1) return result; + await new Promise((r) => setTimeout(r, 30 * (i + 1))); + last = result; + } + return last || { error: 'failed to start proxy' }; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/claude-proxy-adapter.test.mjs` around lines 409 - 417, The current findFreePortForTest() probes and releases a port causing TOCTOU races; instead change the test to remove findFreePortForTest() usage and either (A) call startBuiltinClaudeProxyRuntime with port: 0 (or update runtime to accept port: 0) and assert the returned listenUrl contains the actual bound port, or (B) wrap startBuiltinClaudeProxyRuntime calls in a short retry-on-EADDRINUSE loop (catch EADDRINUSE, wait a few ms, retry) so the runtime binds successfully; update tests that call startBuiltinClaudeProxyRuntime (see invocations around lines where startBuiltinClaudeProxyRuntime is used) to use the chosen approach and remove the separate free-port allocation to eliminate the race window.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/unit/claude-proxy-adapter.test.mjs`:
- Around line 409-417: The current findFreePortForTest() probes and releases a
port causing TOCTOU races; instead change the test to remove
findFreePortForTest() usage and either (A) call startBuiltinClaudeProxyRuntime
with port: 0 (or update runtime to accept port: 0) and assert the returned
listenUrl contains the actual bound port, or (B) wrap
startBuiltinClaudeProxyRuntime calls in a short retry-on-EADDRINUSE loop (catch
EADDRINUSE, wait a few ms, retry) so the runtime binds successfully; update
tests that call startBuiltinClaudeProxyRuntime (see invocations around lines
where startBuiltinClaudeProxyRuntime is used) to use the chosen approach and
remove the separate free-port allocation to eliminate the race window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 141e1b2d-c5ab-4c3c-acdb-ac4aa53b8f7d
📒 Files selected for processing (32)
cli.jscli/claude-proxy.jscli/update.jstests/e2e/test-claude-proxy.jstests/e2e/test-claude.jstests/unit/claude-proxy-adapter.test.mjstests/unit/claude-settings-sync.test.mjstests/unit/config-tabs-ui.test.mjstests/unit/install-target-cards.test.mjstests/unit/provider-share-command.test.mjstests/unit/web-ui-behavior-parity.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/app.jsweb-ui/logic.claude.mjsweb-ui/logic.runtime.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.index.mjsweb-ui/modules/app.methods.install.mjsweb-ui/modules/app.methods.session-actions.mjsweb-ui/modules/app.methods.session-timeline.mjsweb-ui/modules/app.methods.startup-claude.mjsweb-ui/modules/i18n/locales/en.mjsweb-ui/modules/i18n/locales/ja.mjsweb-ui/modules/i18n/locales/vi.mjsweb-ui/modules/i18n/locales/zh.mjsweb-ui/partials/index/layout-header.htmlweb-ui/partials/index/modals-basic.htmlweb-ui/partials/index/panel-config-claude.htmlweb-ui/partials/index/panel-config-codex.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/controls-forms.cssweb-ui/styles/layout-shell.css
💤 Files with no reviewable changes (2)
- web-ui/modules/app.methods.session-timeline.mjs
- web-ui/logic.runtime.mjs
👮 Files not reviewed due to content moderation or server errors (1)
- cli.js
📜 Review details
🔇 Additional comments (23)
web-ui/styles/controls-forms.css (1)
486-486: LGTM!Also applies to: 488-488, 493-495
web-ui/partials/index/panel-config-codex.html (1)
37-37: LGTM!Also applies to: 207-207
cli/update.js (1)
67-104: LGTM!Also applies to: 189-190
web-ui/modules/app.methods.install.mjs (1)
1-3: LGTM!Also applies to: 20-100
web-ui/modules/app.methods.index.mjs (1)
92-92: LGTM!web-ui/partials/index/layout-header.html (1)
113-124: LGTM!web-ui/styles/layout-shell.css (1)
510-563: LGTM!tests/unit/install-target-cards.test.mjs (1)
45-78: LGTM!tests/unit/web-ui-behavior-parity.test.mjs (1)
339-341: LGTM!Also applies to: 375-377, 597-603
web-ui/res/web-ui-render.precompiled.js (1)
166-181: LGTM!Also applies to: 976-976, 1602-1602, 1925-1934, 1938-1953, 5831-5844, 5984-5997
web-ui/modules/app.methods.session-actions.mjs (1)
415-419: LGTM!Also applies to: 423-424
tests/unit/provider-share-command.test.mjs (1)
377-413: LGTM!Also applies to: 429-461
tests/e2e/test-claude.js (1)
1-2: LGTM!Also applies to: 6-6, 48-56, 87-126, 132-150
web-ui/app.js (1)
273-275: LGTM!Also applies to: 283-283, 289-289, 297-299, 562-569, 646-648
web-ui/modules/app.methods.startup-claude.mjs (1)
259-260: LGTM!web-ui/modules/i18n/locales/vi.mjs (1)
109-114: LGTM!Also applies to: 245-253
tests/unit/claude-settings-sync.test.mjs (1)
15-15: LGTM!Also applies to: 335-367, 393-395, 647-651, 712-778, 820-872, 982-984, 1003-1005, 1315-1337
web-ui/logic.claude.mjs (1)
72-78: LGTM!Also applies to: 85-87, 167-169
web-ui/modules/app.methods.claude-config.mjs (1)
26-30: LGTM!Also applies to: 43-45, 64-67, 96-99, 123-127, 154-156, 168-172, 184-185, 198-202, 206-207, 215-218, 241-245, 288-289, 295-298, 317-321
web-ui/modules/i18n/locales/en.mjs (1)
148-153: LGTM!Also applies to: 1146-1153
web-ui/modules/i18n/locales/ja.mjs (1)
149-154: LGTM!Also applies to: 1139-1146
tests/unit/config-tabs-ui.test.mjs (1)
138-140: LGTM!Also applies to: 148-149, 290-305
tests/unit/web-ui-logic.test.mjs (1)
54-55: LGTM!Also applies to: 66-67, 78-79, 84-88, 233-239, 522-533
|
Fixed the refresh/import regression for builtin Claude transform providers. What changed:
Validation run locally:
@coderabbitai review |
|
✅ Action performedFull review finished. |
|
Follow-up pushed in What changed:
Validation actually run locally after this follow-up:
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
Additional browser regression rerun on latest head Browser evidence from an isolated environment:
Post-browser cleanup validation:
|
|
Documentation follow-up pushed in What changed:
Validation actually run for this docs-only follow-up:
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
Follow-up pushed in What changed:
Browser verification actually run:
Validation actually run:
@coderabbitai review |
|
The browser verification with before/after screenshots on both mobile ( ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/update.js`:
- Around line 135-137: The cache check currently only validates TTL so
latestVersionStatusCache can be returned for a different currentVersion; modify
the logic to include the currentVersion in the cache validation: when storing
into latestVersionStatusCache include the input currentVersion (e.g.,
checkedCurrentVersion) and when reading ensure now -
latestVersionStatusCache.checkedAtMs < cacheTtlMs AND
latestVersionStatusCache.checkedCurrentVersion === currentVersion before
returning the cached payload; apply the same change to the other cache-return
site around lines 149-150 so cached results are only reused for the same
currentVersion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17c5ddc8-7319-43bd-8639-a52a90438591
📒 Files selected for processing (17)
cli.jscli/update.jstests/unit/config-tabs-ui.test.mjstests/unit/install-target-cards.test.mjstests/unit/run.mjstests/unit/update-version-status.test.mjstests/unit/web-ui-behavior-parity.test.mjsweb-ui/app.jsweb-ui/modules/app.methods.install.mjsweb-ui/modules/i18n/locales/en.mjsweb-ui/modules/i18n/locales/ja.mjsweb-ui/modules/i18n/locales/vi.mjsweb-ui/modules/i18n/locales/zh.mjsweb-ui/partials/index/layout-header.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/layout-shell.cssweb-ui/styles/responsive.css
✅ Files skipped from review due to trivial changes (7)
- web-ui/styles/responsive.css
- tests/unit/update-version-status.test.mjs
- web-ui/modules/i18n/locales/ja.mjs
- web-ui/modules/i18n/locales/vi.mjs
- web-ui/modules/i18n/locales/en.mjs
- web-ui/modules/i18n/locales/zh.mjs
- web-ui/res/web-ui-render.precompiled.js
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/unit/config-tabs-ui.test.mjs
- web-ui/styles/layout-shell.css
- tests/unit/web-ui-behavior-parity.test.mjs
- web-ui/app.js
- web-ui/modules/app.methods.install.mjs
- cli.js
📜 Review details
🔇 Additional comments (4)
cli/update.js (1)
106-126: LGTM!Also applies to: 237-240
tests/unit/install-target-cards.test.mjs (1)
49-155: LGTM!tests/unit/run.mjs (1)
57-57: LGTM!web-ui/partials/index/layout-header.html (1)
2-13: LGTM!Also applies to: 126-130
| if (latestVersionStatusCache && cacheTtlMs > 0 && now - latestVersionStatusCache.checkedAtMs < cacheTtlMs) { | ||
| return { ...latestVersionStatusCache.payload, cached: true }; | ||
| } |
There was a problem hiding this comment.
Cache payload is reused across different currentVersion inputs.
At Line 135, cache reuse only checks TTL. A call for version A can incorrectly return cached payload for version B within TTL, including wrong currentVersion and updateAvailable.
Suggested fix
let latestVersionStatusCache = null;
async function fetchLatestVersionStatus(options = {}) {
const currentVersion = normalizePackageVersion(options.currentVersion) || String(options.currentVersion || '');
+ const cacheKey = currentVersion;
const timeoutMs = Number.isFinite(Number(options.timeoutMs)) ? Number(options.timeoutMs) : 5000;
const cacheTtlMs = Number.isFinite(Number(options.cacheTtlMs)) ? Math.max(0, Number(options.cacheTtlMs)) : 10 * 60 * 1000;
const now = typeof options.now === 'function' ? options.now() : Date.now();
- if (latestVersionStatusCache && cacheTtlMs > 0 && now - latestVersionStatusCache.checkedAtMs < cacheTtlMs) {
+ if (
+ latestVersionStatusCache &&
+ latestVersionStatusCache.cacheKey === cacheKey &&
+ cacheTtlMs > 0 &&
+ now - latestVersionStatusCache.checkedAtMs < cacheTtlMs
+ ) {
return { ...latestVersionStatusCache.payload, cached: true };
}
@@
- latestVersionStatusCache = { checkedAtMs: now, payload };
+ latestVersionStatusCache = { checkedAtMs: now, cacheKey, payload };
return payload;
}Also applies to: 149-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/update.js` around lines 135 - 137, The cache check currently only
validates TTL so latestVersionStatusCache can be returned for a different
currentVersion; modify the logic to include the currentVersion in the cache
validation: when storing into latestVersionStatusCache include the input
currentVersion (e.g., checkedCurrentVersion) and when reading ensure now -
latestVersionStatusCache.checkedAtMs < cacheTtlMs AND
latestVersionStatusCache.checkedCurrentVersion === currentVersion before
returning the cached payload; apply the same change to the other cache-return
site around lines 149-150 so cached results are only reused for the same
currentVersion.
|
Version follow-up pushed in What changed:
Verification actually run before push:
@coderabbitai review |
|
✅ Action performedFull review finished. |
|
Version correction pushed in What changed:
Verification actually run before push:
Note: npm latest is still @coderabbitai review |
|
✅ Action performedFull review finished. |
awsl233777
force-pushed
the
feat/claude-chat-completions-proxy
branch
from
b4d0ab3 to
e62ffc0
Compare
|
Cleanup pushed in What changed after the previous version correction:
Validation already run before this squash (tree content unchanged except commit history/tag pointer):
@coderabbitai review |
|
✅ Action performedFull review finished. |
|
Release workflow fix pushed in What changed:
Validation actually run:
@coderabbitai review |
|
The live release at 🧠 Learnings used✅ Action performedFull review finished. |
2 participants
Summary
/v1/modelsand/v1/messagesthrough the selected upstream API, including Anthropic → Ollama/api/chatconversion.Real endpoint validation
Validated the Ollama target mode against a provided real Ollama endpoint without writing to the user's real
~/.codexconfig:targetApi: 'ollama'and a direct upstream base URL.GET /v1/modelsrouted through the proxy to upstream/api/tagsand returned the expected Ollama cloud model list.POST /v1/messagesrouted through the proxy to upstream/api/chatand converted the response back into an Anthropic-style message:minimax-m3:cloudsucceeded with a non-empty response whenmax_tokenswas raised to 128.gemma4:31b-cloudsucceeded with a non-empty response.Observed edge case:
minimax-m3:cloudcan return onlymessage.thinkingwith an emptymessage.contentwhenmax_tokens/ Ollamanum_predictis too small (for example 32 with a system prompt). This is upstream model behavior; practical configs should avoid very small max token limits for that model.Validation
zip-lib; that failure was an environment dependency issue, not an Ollama proxy-path failure.Summary by CodeRabbit
New Features
Improvements
Tests